feat: custom exception handler #6710

kenjis · 2022-10-18T07:30:00Z

Description
Supersedes #5675

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

MGatner

This is a big one, I will have to come back to it! Initially it looks really good. Please be sure to work with @lonnieezell for feature consistency and @paulbalandan if there's any collaboration to be had with the deprecations handler PR.

system/Debug/BaseExceptionHandler.php

lonnieezell · 2022-10-18T13:46:55Z

I'll be honest - this week I'm completely wrecked for time so I won't be able to really review until next week.

That said - the reason nothing happened with the original one was because it was a BC break and would have to wait until v5. Has the BC break portion been fixed with this PR?

kenjis · 2022-10-18T20:57:22Z

No problem. The original PR was created many months ago.

I have tried to keep this PR free of BC breaks.

kenjis · 2022-10-23T07:37:07Z

Rebased.

MGatner

I haven't looked at docs and tests yet, but this is looking great! I added some comments but two broad thoughts:

It seems like the new classes are independent of the existing ones, and you use the current handle as a mediator to call them when available. I like the freedom that gives us - but then you kept ExceptionHandler compatible with the current class. It don't understand the need for this backwards-compatibility, unless I am missing something?
I know we've been burned by class-interface-abstraction, but enforcing our BaseExceptionHandler for a single handle() method feels unduly restrictive. I would like to see an interface (just that method) used as the return from the Config file and the abstract class becomes optional.

system/Debug/BaseExceptionHandler.php

MGatner · 2022-10-25T10:35:11Z

system/Debug/BaseExceptionHandler.php

+     *
+     * @return bool|string
+     */
+    protected static function highlightFile(string $file, int $lineNumber, int $lines = 15)


Since the existing handler doesn't actually extend this base, can we go ahead and type this?

What do you mean?

MGatner · 2022-10-25T10:39:21Z

system/Debug/Exceptions.php

@@ -122,6 +125,23 @@ public function exceptionHandler(Throwable $exception)
            ]);
        }

+        // For upgraded users.
+        if (! method_exists($this->config, 'handler')) {
+            $this->defaultExceptionHandler($exception, $statusCode, $exitCode);


Why doesn't this use ExceptionHandler? Is it not backwards-compatible behavior?

Good point.
It should use ExceptionHandler.
When using ExceptionHandler, if the behavior changes it is a breaking change.

system/Debug/ExceptionHandler.php

MGatner · 2022-10-25T10:42:50Z

system/Debug/ExceptionHandler.php

+    /**
+     * Determines the correct way to display the error.
+     *
+     * @return void


It feels like a big loss not having this in the abstraction. Any reason not to do that?

The section "Error Handling" has been added.

kenjis · 2022-10-27T08:42:12Z

No. If a dev overrides the following methods, this PR breaks the extended Exceptions:

Exceptions::render()
Exceptions::determineView()
Exceptions::collectVars()
Exceptions::maskSensitiveData()

kenjis · 2022-10-27T08:53:12Z

Reverting 9d5d44f and a dev does not update the Config class,
then there is no BC break even if the dev extends Exceptions.

MGatner · 2022-10-27T10:20:10Z

I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?

MGatner · 2022-10-27T10:27:54Z

No. If a dev overrides the following methods, this PR breaks the extended Exceptions

If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.

kenjis · 2022-10-27T12:19:43Z

I'm confused; I think I missed something. ExceptionHandler and BaseExceptionHandler are new classes, and Exceptions is not being changed to extend either. So why do the new classes need to be backwards-compatible with Exceptions?

The new classes do not need to be backwards-compatible with Exceptions.

A dev can replace Exceptions with their custom extended class now, and if the dev overrides one of some methods,
this PR breaks the custom Exceptions.

If we want to support extended classes just check if (static::class === self::class) before passing off to the handler.

It may be Okay with that check, as a dev would not need to use their own ExceptionHandler and replace Exceptions at the same time.
Alternatively, it can also be determined by setting it to opt-in in the Config.

lonnieezell

Looks pretty good to me. I just found one edge case that we don't handle.

lonnieezell · 2022-10-27T17:50:06Z

system/Debug/ExceptionHandler.php

+        $viewFile = null;
+        if (is_file($path . $view)) {
+            $viewFile = $path . $view;
+        } elseif (is_file($altPath . $altView)) {


Seems like we need to handle if neither file exists?

MGatner · 2022-10-28T10:42:15Z

The new classes do not need to be backwards-compatible with Exceptions.

Since this is the case please add explicit types to all the new class methods.

I am also still keen on adding an interface - nothing else in the base class is mandatory so we shouldn't restrict users to extending it.

kenjis added enhancement PRs that improve existing functionalities 4.3 labels Oct 18, 2022

kenjis requested a review from lonnieezell October 18, 2022 07:32

MGatner reviewed Oct 18, 2022

View reviewed changes

system/Debug/BaseExceptionHandler.php Outdated Show resolved Hide resolved

kenjis requested a review from paulbalandan October 18, 2022 11:38

kenjis force-pushed the feat-ExceptionHandler branch from b22c181 to 2017313 Compare October 23, 2022 07:32

kenjis force-pushed the feat-ExceptionHandler branch 2 times, most recently from af3f99e to 9db24aa Compare October 24, 2022 23:37

MGatner requested changes Oct 25, 2022

View reviewed changes

kenjis force-pushed the feat-ExceptionHandler branch from 9db24aa to b93407c Compare October 26, 2022 09:45

kenjis added 16 commits October 27, 2022 17:21

feat: add Debug\ExceptionHandler

54cc18b

refactor: use ExceptionHandler

ebf59e8

docs: add user guide

a7fb791

fix: cannot override $viewPath

45fe5a7

refactor: run rector & cs-fixer

fa31541

chore: update test-phpcpd.yml

9d724dc

refactor: run rector

550e62e

fix: set deprecated properties for backward compatibility

7777772

docs: add Deprecations

038717a

refactor: move ResponseTrait to ExceptionHandler

4a8bc0d

docs: add @deprecated

30133af

docs: fix changelog

d1e7678

The section "Error Handling" has been added.

docs: add @Used-By

f41f2ef

refactor: use number helper

a610d48

refactor: make final class

50b02c8

refactor: remove defaultExceptionHandler() and use ExceptionHandler

9d5d44f

kenjis force-pushed the feat-ExceptionHandler branch from b93407c to 9d5d44f Compare October 27, 2022 08:21

kenjis added the breaking change Pull requests that may break existing functionalities label Oct 27, 2022

refactor: run rector

dc9fc53

lonnieezell reviewed Oct 27, 2022

View reviewed changes

kenjis added 4.4 and removed 4.3 labels Oct 28, 2022

MGatner mentioned this pull request Oct 30, 2022

Turn on logging for deprecation notices by default #6773

Merged

5 tasks

kenjis deleted the branch codeigniter4:4.3 January 10, 2023 06:36

kenjis closed this Jan 10, 2023

kenjis mentioned this pull request Jan 11, 2023

feat: custom exception handler #7087

Merged

5 tasks

kenjis deleted the feat-ExceptionHandler branch February 8, 2023 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom exception handler #6710

feat: custom exception handler #6710

kenjis commented Oct 18, 2022 •

edited

Loading

MGatner left a comment

lonnieezell commented Oct 18, 2022

kenjis commented Oct 18, 2022

kenjis commented Oct 23, 2022

MGatner left a comment

MGatner Oct 25, 2022 •

edited

Loading

kenjis Oct 27, 2022

MGatner Oct 25, 2022

kenjis Oct 26, 2022

kenjis Oct 27, 2022

MGatner Oct 25, 2022

kenjis commented Oct 27, 2022

kenjis commented Oct 27, 2022

MGatner commented Oct 27, 2022

MGatner commented Oct 27, 2022

kenjis commented Oct 27, 2022

lonnieezell left a comment

lonnieezell Oct 27, 2022

MGatner commented Oct 28, 2022

feat: custom exception handler #6710

feat: custom exception handler #6710

Conversation

kenjis commented Oct 18, 2022 • edited Loading

MGatner left a comment

Choose a reason for hiding this comment

lonnieezell commented Oct 18, 2022

kenjis commented Oct 18, 2022

kenjis commented Oct 23, 2022

MGatner left a comment

Choose a reason for hiding this comment

MGatner Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

kenjis Oct 27, 2022

Choose a reason for hiding this comment

MGatner Oct 25, 2022

Choose a reason for hiding this comment

kenjis Oct 26, 2022

Choose a reason for hiding this comment

kenjis Oct 27, 2022

Choose a reason for hiding this comment

MGatner Oct 25, 2022

Choose a reason for hiding this comment

kenjis commented Oct 27, 2022

kenjis commented Oct 27, 2022

MGatner commented Oct 27, 2022

MGatner commented Oct 27, 2022

kenjis commented Oct 27, 2022

lonnieezell left a comment

Choose a reason for hiding this comment

lonnieezell Oct 27, 2022

Choose a reason for hiding this comment

MGatner commented Oct 28, 2022

kenjis commented Oct 18, 2022 •

edited

Loading

MGatner Oct 25, 2022 •

edited

Loading